Decouple auth from http#468
Conversation
JordonPhillips
force-pushed
the
no-http-auth
branch
2 times, most recently
from
4356949 to
fb5bf53
Compare
JordonPhillips
force-pushed
the
no-http-auth
branch
from
fb5bf53 to
194c334
Compare
nateprewitt
left a comment
nateprewitt
left a comment
There was a problem hiding this comment.
Need to do one more pass tomorrow, but looks good. Just one question on why we're making some of the types private.
JordonPhillips
changed the title
JordonPhillips
force-pushed
the
no-http-auth
branch
from
7ad2adf to
6f474c6
Compare
JordonPhillips
force-pushed
the
no-http-auth
branch
2 times, most recently
from
dbae11e to
6dbca91
Compare
|
|
||
| Signers are constructed by the `AuthScheme`'s `signer` method. | ||
|
|
||
| Signers MAY modify the given request and return it, or construct a new signed |
There was a problem hiding this comment.
Signers MAY modify the given request and return it
If we're recommending this, do we know how it's going to interact with retries for generic clients? I think that's the primary concern prompting creating a new one.
There was a problem hiding this comment.
We already will create a copy of the request before each attempt, so the potential ramifications are limited.
| expiration: datetime | None = None | ||
| """The expiration time of the identity. | ||
|
|
||
| If time zone is provided, it is updated to UTC. The value must always be in UTC. |
There was a problem hiding this comment.
We say this in a bunch of places but I'm not sure I've seen an implementation where we actually do this. Is this a note to implementers, or should we have some base implementation that checks timezones and makes adjustments?
There was a problem hiding this comment.
The identity resolver is expected to do that. I could update this to a dataclass and drop it in post_init?
There was a problem hiding this comment.
That seems reasonable if we have a base implementation. Otherwise, it may be worth simplifying the wording to just "All date times with a timezone must be in UTC."
This decouples auth from HTTP, allowing it to be defined centrally. A number of changes have been made to various interfaces. Notably identity resolvers are generally expected to have zero-arg constructors, instead getting everything they need from their properties where possible. Construction of event signers has been moved into AuthScheme. Right now it takes the entire context of the request during construction, but we should consider passing identity and signing properties in at signing time like normal signers so that they can be reused.
JordonPhillips
force-pushed
the
no-http-auth
branch
from
75e5630 to
f00644a
Compare
| expiration: datetime | None = None | ||
| """The expiration time of the identity. | ||
|
|
||
| If time zone is provided, it is updated to UTC. The value must always be in UTC. |
There was a problem hiding this comment.
That seems reasonable if we have a base implementation. Otherwise, it may be worth simplifying the wording to just "All date times with a timezone must be in UTC."
2 participants
This decouples auth from HTTP, allowing it to be defined centrally.
A number of changes have been made to various interfaces. Notably identity resolvers are generally expected to have zero-arg constructors, instead getting everything they need from their properties where possible.
Construction of event signers has been moved into AuthScheme.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.